-
Notifications
You must be signed in to change notification settings - Fork 720
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
wasm2c: Implement EHv4 #2513
wasm2c: Implement EHv4 #2513
Conversation
@sbc100 (this might still have some nits? can't remember. something about updating comments?) |
@sbc100 should be good now |
hello @shravanrn @keithw @sbc100 can anyone take a look at this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
lgtm % comments
wasm2c/wasm-rt-impl-tableops.inc
Outdated
#if defined(WASM_RT_TABLE_OPS_FUNCREF) + \ | ||
defined(WASM_RT_TABLE_OPS_EXTERNREF) + \ | ||
defined(WASM_RT_TABLE_OPS_EXNREF) > \ | ||
1 | ||
#error \ | ||
"Expected only one of { WASM_RT_TABLE_OPS_FUNCREF, WASM_RT_TABLE_OPS_EXTERNREF } to be defined" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error should now include WASM_RT_TABLE_OPS_EXNREF?
/** | ||
* The maximum size of an exception. | ||
*/ | ||
#define WASM_EXN_MAX_SIZE 256 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In practice I think this will normally just be a 4-byte pointer. So maybe 32 is enough for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
256 is what we use today, so we just used that. we would prefer not to break compatibility with existing wasm2c exception handling users (assuming we have some).
src/c-writer.cc
Outdated
@@ -635,6 +638,19 @@ void CWriter::PopLabel() { | |||
label_stack_.pop_back(); | |||
} | |||
|
|||
// static | |||
constexpr bool CWriter::HasNonNullInitializers(Type type) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is always using in the negative how about inverting it? i.e. "NullOnlyInitializers"?
Continuation of #2470 / #2512